[jsweep] Clean validate_context_variables.cjs#38785
Conversation
- Remove unnecessary try/catch that caused setFailed to be called twice on validation failure (first with detailed message, then in catch with ERR_VALIDATION prefix, producing 'Context variable validation failed: - Include ERR_VALIDATION prefix directly in the single setFailed call - Extract failureDetails variable for clarity in error message construction - Remove unused getErrorMessage import from error_helpers.cjs - Add 7 new tests covering: ERR_VALIDATION prefix in error message, single setFailed call assertion, empty context (0 fields checked), correct validated count reporting, multiple simultaneous failures, run_id/run_number top-level validation, and run_id injection rejection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38785 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (well under the 100-line threshold; requires_adr_by_default_volume=false). Neither Condition A nor Condition B is met. |
There was a problem hiding this comment.
Pull request overview
This PR cleans up the GitHub Actions github-script helper actions/setup/js/validate_context_variables.cjs to avoid calling core.setFailed() twice on validation failures, ensuring the final failure message is not overwritten and does not contain duplicated “Context variable validation failed” text.
Changes:
- Removed the redundant try/catch wrapper so validation failures produce a single
setFailedcall with a stable message. - Included the
ERR_VALIDATIONprefix directly in the constructed validation failure message and extractedfailureDetailsfor readability. - Added targeted tests to verify single-failure behavior, correct message prefixing, validated-count reporting, and top-level
run_id/run_numbervalidation.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/validate_context_variables.cjs | Removes redundant error handling and constructs a single, prefixed failure message for numeric context validation. |
| actions/setup/js/validate_context_variables.test.cjs | Adds coverage to ensure one setFailed call, correct ERR_VALIDATION prefixing, accurate validated counts, and run_id/run_number checks. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics & Test Classification (43 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 82/100. Test quality is excellent — 2.3% of new tests are implementation tests (threshold: 30%). 43 tests analyzed: 42 design, 1 implementation, 0 guideline violations. Strong coverage of security-critical rejection cases, boundary values, and multi-failure error paths.
|
@copilot summarize any remaining blockers and confirm merge readiness.
|
|
``
|
Summary
Cleans
actions/setup/js/validate_context_variables.cjsby removing a redundant try/catch that causedcore.setFailedto be called twice on validation failure.Context Type
This file runs in github-script context — it uses
core.info(),core.error(),core.setFailed()and receivescontextas a global.Changes Made
Bug Fix: Double
setFailedcall removedThe old
validateContextVariableswrapped all logic in a try/catch where:setFailed(detailedMessage)called, thenthrow new Error(detailedMessage)setFailed(ERR_VALIDATION + ": Context variable validation failed: " + detailedMessage)— overwriting the first callThe final error seen by GitHub Actions was
ERR_VALIDATION: Context variable validation failed: Context variable validation failed!\n\nFound...— the phrase "Context variable validation failed" appeared twice.After cleanup
ERR_VALIDATIONprefix is now included directly in the singlesetFailedcallfailureDetailsvariable for clarity in error message constructiongetErrorMessageimport fromerror_helpers.cjsTest Improvements
7 new tests added (36 → 43 total):
setFailedcalled exactly once (not twice)$(curl evil.example.com)blockedValidation Checks ✅
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓ (file already had@ts-check)npm run test:js✓ — 43/43 tests pass